Conversation
…lure
busybox's flock applet has no -w option, but because it installs a flock
symlink to /bin/busybox, `command -v flock` succeeds on Alpine. The
"flock is unavailable, skip locking" guards therefore never fired and
the lock call itself was what failed.
Two of the five call sites then matched `|| { ...; return 0; }` and
reported success while writing nothing: flush_traffic_to_disk() dropped
the counters, and save_traffic() left the Telegram daemon persisting no
traffic at all. Both were completely silent. The other three sites
aborted with "Could not acquire traffic lock", which made traffic resets,
replication saves and monthly quota resets impossible on Alpine.
Probe for -w support once and fall back to a non-blocking acquire, so the
lock is still taken; and make the two fail-open paths report failure
instead of claiming success.
Verified on Alpine 3.20: tests/test_traffic_reset.sh goes from 21
failures to 0, and the new tests/test_flock_portability.sh pins both the
busybox behaviour and the fail-closed behaviour on every platform.
The fail-closed paths this PR adds could not be heard. `exec` with only redirections applies to the whole shell, so `exec 9>&- 2>/dev/null` leaves fd 2 pointing at /dev/null for the rest of the process — not just for that line. In save_replication the log_error added here writes to >&2 and was therefore swallowed by the line immediately above it. Three of these were introduced by this PR (two fd 9 closes in the new fail-closed branches, one fd 201 close); the other three are the same idiom already on main, in functions this PR rewrites. All six are fixed so the failure paths actually report something. Verified: after `exec 201>file || true`, fd 2 still points at the caller's stderr rather than /dev/null, and the flock suite still passes on both Debian 12 and Alpine 3.20.
rvalitov
marked this pull request as draft
September 17, 2026 18:52
The helpers added for busybox flock were defined only in the manager
script, but the bot daemon is emitted from a single-quoted heredoc and
never sources the manager -- as its own header comment states. The
generated daemon therefore called an undefined _lock_fd: save_traffic()
got 127 back, took the new fail-closed branch and returned 1 without
writing a single counter. Parsing the heredoc out of mtproxymax.sh yields
1052 lines with one call site and no definition of either helper.
Because an undefined command fails on every host, this did not merely
leave the Alpine bug unfixed: it broke accounting on util-linux too, where
the previous `flock -w 5 9 ... || { ...; return 0; }` had worked. The
daemon is the only writer of accounting in production, so counters stopped
being persisted everywhere.
Copy both helpers into the heredoc next to save_traffic(), and stop
redirecting the update_traffic call to /dev/null so that a lock failure
reaches the log instead of being discarded.
tests/test_flock_portability.sh only ever exercised the manager, which is
why CI stayed green. It now extracts the generated daemon, asserts that it
defines the helpers it calls, and runs the daemon's own save_traffic under
a busybox-style flock.
Verified: the new daemon cases fail 7/16 against the previous revision of
mtproxymax.sh and pass 16/16 with it; the util-linux, held-lock and
no-flock branches all behave as before.
rvalitov
marked this pull request as ready for review
September 17, 2026 20:19
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
busybox ships a
flockapplet that has no-woption. Because it installs aflocksymlink to/bin/busybox,command -v flocksucceeds on Alpine — so the "flock is unavailable, skip locking" guards never fired, and it was the lock call itself that failed:Five call sites pass
-w 5. Three aborted with "Could not acquire traffic lock", making all of these impossible on Alpine:secret_reset_traffic—secret reset-traffic, and monthly quota resets (which call it with output discarded, so they failed silently)run_traffic_reset_global— global traffic resetsave_replication— replication config was never savedThe other two were worse: they had no guard at all and matched
|| { exec 9>&-; return 0; }, so they reported success while writing nothing.flush_traffic_to_disk— counters silently discarded before stop/restartsave_traffic— the Telegram daemon persisted no traffic at all, with no error anywhereFix
Probe for
-wsupport once, and fall back toflock -n, which busybox does support. The lock is still taken; only the 5-second grace period is lost. On a host with noflockat all, behaviour is unchanged (proceed unlocked)._lock_fdcentralises all five sites, and the two fail-open paths now return non-zero instead of 0. Those two write to stderr rather thanlog_errorbecausesave_trafficalso runs inside the generated daemon script, which has no logging helpers.Testing
New
tests/test_flock_portability.shplaces a busybox-styleflockonPATH, so the regression is caught on every platform rather than only on Alpine. It also pins the fail-closed behaviour.Verified on Alpine 3.20:
tests/test_traffic_reset.shgoes from 21 failures to 0. No test result changes on Debian 12, Ubuntu 22.04/24.04 or Fedora 41.